Skip to content

test(build): drop brittle exact dnf5 call-count assertion - #355

Open
agriffis wants to merge 1 commit into
projectbluefin:mainfrom
agriffis:fix/354-relax-dnf5-call-count-assertion
Open

agriffis wants to merge 1 commit into
projectbluefin:mainfrom
agriffis:fix/354-relax-dnf5-call-count-assertion

Conversation

@agriffis

Copy link
Copy Markdown

Fixes #354

The 10-build: installs the packages the default ujust recipes depend on test asserted exactly one dnf5 call for the baseline install -y tmux gum line. Forks following the documented packages/COPR customization guidance add extra dnf5 calls (a single copr_install_isolated call alone contributes 3 more: copr enable, copr disable, install --enablerepo=...), which broke this test even though the resulting build was correct.

This drops the exact-count assertion and keeps only the check that the baseline install runs (and is first).

Assisted-by: Claude Opus 4.5 via pi

The default-recipe-dependencies test asserted exactly one dnf5 call
for the baseline `install -y tmux gum` line. Forks following the
documented packages/COPR customization guidance add extra dnf5 calls
(a single copr_install_isolated call alone contributes 3 more), which
broke this test even though the resulting build was correct.

Keep only the check that the baseline install is first, dropping the
exact count assertion.

Fixes projectbluefin#354

Assisted-by: Claude Opus 4.5 via pi

@hanthor hanthor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remaining assertion still has teeth — but the same pattern survives four lines below

The change is safe

My first concern with "drop an assertion to fix a test" is that the test stops testing anything. It does not here. I removed the code under test and confirmed the test still catches it:

$ git checkout test/355          # this branch merged into origin/main
$ sed -i 's/^dnf5 install -y tmux gum$/: removed/' build/10-build.sh
$ bats tests/unit/10-build_test.bats
not ok 9 10-build: installs the packages the default ujust recipes depend on

With the install line gone, calls[0] is empty and [ "${calls[0]}" = "install -y tmux gum" ] fails. The surviving assertion is load-bearing, and it still pins the stronger property — that the baseline install is first, not merely present.

Gates on this branch merged into origin/main @ 1db684b: bats tests/unit 180/180 ok, just lint exit 0, just check exit 0 — identical to the baseline I measured on unmodified main.

The premise also holds: this is a template repo whose own docs tell forks to add packages and COPRs to build/10-build.sh, and copr_install_isolated emits three dnf5 calls per invocation (verified by copr_install_isolated: enables, disables, then installs from the repo id in copr-helpers_test.bats). An exact count of 1 is genuinely hostile to the documented customization path.

The inconsistency

The very next test in the same file has the identical problem and is untouched:

@test "10-build: enables exactly the podman and brew units" {
    ...
    mapfile -t calls <"${SYSTEMCTL_LOG}"
    [ "${#calls[@]}" -eq 4 ]              # <-- same brittleness, still here
    [ "${calls[0]}" = "enable podman.socket" ]
    [ "${calls[1]}" = "enable brew-setup.service" ]
    ...
}

A fork that enables one extra systemd unit — which is at least as common as adding a package — breaks on -eq 4 for exactly the reason #354 describes for -eq 1. If the principle is "exact call counts are wrong in a template repo", it should be applied to both; if the principle is narrower than that, the commit message should say what makes the dnf5 case special. As it stands the next person to hit this files #356 and we do this again.

My suggestion: either extend this PR to the systemctl case, or keep the counts in both and change them to lower bounds plus ordered-prefix assertions, e.g.

[ "${#calls[@]}" -ge 1 ]
[ "${calls[0]}" = "install -y tmux gum" ]

which preserves "nothing runs before the baseline install" while tolerating fork additions. That is strictly more information than dropping the count outright.

Blocker

No CI has run on this PR. get_check_runs returns total_count: 0, and the run is parked awaiting fork-workflow approval:

Unit Tests, run_number 57, head_sha a28f676
  event: pull_request, status: completed, conclusion: action_required

Needs a maintainer to approve before BATS unit tests can report.


Generated by Claude Code

@agriffis

Copy link
Copy Markdown
Author

Yeah, I'd agree with that. As written, these particular tests preclude customization rather than planning for it.

@agriffis

Copy link
Copy Markdown
Author

Just a heads up, even though I think this PR is on the right track, I realized I don't care about it. Bluefin is transitioning from a Fedora base to GNOME OS, and the stuff in this repo doesn't seem to represent the future state, unless I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10-build unit test asserts exact dnf5 call count, breaking on any COPR or extra install

2 participants